-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Allow single char version slugs. #1407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -30,7 +30,7 @@ | |||
# +? -- allow multiple of those, but be not greedy about the matching | |||
# (?: ... ) -- wrap everything so that the pattern cannot escape when used in | |||
# regexes. | |||
VERSION_SLUG_REGEX = '(?:[a-z0-9][-._a-z0-9]+?)' | |||
VERSION_SLUG_REGEX = '(?:[a-z0-9][-._a-z0-9]*?)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the final ?
is not required here. *
should catch none/any number of instances of allowed characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial intend for adding the "non-greedy" matcher was that the regex does not leak into some other bits of a URL regex when it is embedded into one. But, yeah, I think it won't make any difference in real use. Shall I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, I figured there were some other cases to consider here. Should be fine leaving it
6bb2b94
to
212afb0
Compare
212afb0
to
e557a20
Compare
Seems like an edge case (in fact, it could only happen 26 times :) -- Perhaps we should set slugging rules, and require 3+ chars? At this size names/slugs are meaningless -- We've also run into issues in the past with UTF8 chars getting slugified poorly (eg. chinese chars will slug down to nothing). That might be another odd edge case that this code will hit. |
Opened #1410 to address UTF8 encoding on slugging. This looks good otherwise |
Allow single char version slugs.
The version slug regex was previously not allowing single char slugs. This PR fixes that and is adding appropriate tests.
Closes #1405.